Silence -Wunused-parameter warnings in Unwind-wasm.c#125412
Silence -Wunused-parameter warnings in Unwind-wasm.c#125412
Conversation
|
@llvm/pr-subscribers-libunwind Author: Yuriy Chernyshov (georgthegreat) ChangesFull diff: https://github.com/llvm/llvm-project/pull/125412.diff 1 Files Affected:
diff --git a/libunwind/src/Unwind-wasm.c b/libunwind/src/Unwind-wasm.c
index b18b32c5d17847..fe016b8f5923ad 100644
--- a/libunwind/src/Unwind-wasm.c
+++ b/libunwind/src/Unwind-wasm.c
@@ -102,8 +102,8 @@ _LIBUNWIND_EXPORT uintptr_t _Unwind_GetIP(struct _Unwind_Context *context) {
}
/// Not used in Wasm.
-_LIBUNWIND_EXPORT void _Unwind_SetIP(struct _Unwind_Context *context,
- uintptr_t value) {}
+_LIBUNWIND_EXPORT void _Unwind_SetIP([[maybe_unused]] struct _Unwind_Context *context,
+ [[maybe_unused]] uintptr_t value) {}
/// Called by personality handler to get LSDA for current frame.
_LIBUNWIND_EXPORT uintptr_t
@@ -116,7 +116,7 @@ _Unwind_GetLanguageSpecificData(struct _Unwind_Context *context) {
/// Not used in Wasm.
_LIBUNWIND_EXPORT uintptr_t
-_Unwind_GetRegionStart(struct _Unwind_Context *context) {
+_Unwind_GetRegionStart([[maybe_unused]] struct _Unwind_Context *context) {
return 0;
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
f3f30cc to
69307d5
Compare
MaskRay
left a comment
There was a problem hiding this comment.
What's your build option?
llvm-project considers this warning low value and llvm/cmake/modules/HandleLLVMOptions.cmake specifies -Wno-unused-parameter.
|
@MaskRay, we build libunwind using our own build system with the clang default set of warnings enabled. There is already a couple of |
ldionne
left a comment
There was a problem hiding this comment.
IMO this is reasonable but it's simpler to drop the parameter names in this case, no need for [[maybe_unused]].
Co-authored-by: Louis Dionne <ldionne.2@gmail.com>
|
I applied @ldionne suggestions, I am fine with his approach. |
|
@ldionne, may we proceed with merging this? |
|
Sure, but I am not certain why you added some newlines though. Can you leave the formatting as it was? |
749510a to
c064b57
Compare
|
I think this is how my clang-format-16 auto-formatted it. |
|
With these parameters gone, now we have this warning: ( .../libunwind/src/Unwind-wasm.c:105:62: error: omitting the parameter name in a function
definition is a C23 extension [-Werror,-Wc23-extensions]
105 | _LIBUNWIND_EXPORT void _Unwind_SetIP(struct _Unwind_Context *, uintptr_t) {}
| |
llvm/llvm-project#125412 removed parameter names to suppress `-Wunused-parameter` in their own build system. As a result now we have this error: ```console ../../../system/lib/libunwind/src/Unwind-wasm.c:105:62: error: omitting the parameter name in a function definition is a C23 extension [-Werror,-Wc23-extensions] 105 | _LIBUNWIND_EXPORT void _Unwind_SetIP(struct _Unwind_Context *, uintptr_t) {} | ``` We have three ways to fix it? 1. Re-add parameter names. But this will later clash with llvm/llvm-project#125412. 2. Add `-std=c23` to the cflags. But this only applies to .c files and our `get_cflags` function in `system_libs.py` doesn't take a file name so we can't apply different cflags depending on the file extensions. While it's possible to add the filename paramater to `get_cflags`, it is a bigger refactoring. 3. Suppress the warning using `-Wno-c23-extension`. This does 3, which is the quickest. But eventually we may need to change what llvm/llvm-project#125412 did.
|
It seems that libunwind builds with C++17: llvm-project/libunwind/src/CMakeLists.txt Lines 139 to 145 in 3bf5384 Which means this change actually breaks the build. We were about to bump to C++23 downstream but that seems like the wrong solution if upstream is on C++17: emscripten-core/emscripten#26037 (comment) If this analysis is correct then I propose this change is reverted, and that the revert is backported to LLVM 21. |
|
In several other places in libunwind it looks like more basic methods of suppressing this warning are used: llvm-project/libunwind/src/AddressSpace.hpp Lines 465 to 466 in 3bf5384 |
|
I confirmed the LLVM builds these files with @georgthegreat I suggest you update your downstream build system to match the llvm flags, otherwise you will likely run into more issues like that. |
This updates libunwind from 20.1.8 to LLVM 21.1.8: https://github.com/llvm/llvm-project/releases/tag/llvmorg-21.1.8 Additional change: - Add `-Wno-c23-extensions` to libunwind's cflags: 0070206 llvm/llvm-project#125412 removed parameter names to suppress `-Wunused-parameter` in their own build system, but this causes these errors instead for us: ```console ../../../system/lib/libunwind/src/Unwind-wasm.c:105:62: error: omitting the parameter name in a function definition is a C23 extension [-Werror,-Wc23-extensions] 105 | _LIBUNWIND_EXPORT void _Unwind_SetIP(struct _Unwind_Context *, uintptr_t) {} | ``` This suppresses the warnings.
This updates libunwind from 20.1.8 to LLVM 21.1.8: https://github.com/llvm/llvm-project/releases/tag/llvmorg-21.1.8 Additional change: - Add `-Wno-c23-extensions` to libunwind's cflags: emscripten-core@0070206 llvm/llvm-project#125412 removed parameter names to suppress `-Wunused-parameter` in their own build system, but this causes these errors instead for us: ```console ../../../system/lib/libunwind/src/Unwind-wasm.c:105:62: error: omitting the parameter name in a function definition is a C23 extension [-Werror,-Wc23-extensions] 105 | _LIBUNWIND_EXPORT void _Unwind_SetIP(struct _Unwind_Context *, uintptr_t) {} | ``` This suppresses the warnings.
Reverts #125412 See the discussion in #125412 for why this is necessary. The summary is that: - Eliding arguments is the C23 extension, but libunwind builds its C files with `-std=c99`, so this change broke the build. - `-Wno-unused-parameter` is part of the build for libunwind, so the codebase does allow them.
….c" (#175776) Reverts llvm/llvm-project#125412 See the discussion in #125412 for why this is necessary. The summary is that: - Eliding arguments is the C23 extension, but libunwind builds its C files with `-std=c99`, so this change broke the build. - `-Wno-unused-parameter` is part of the build for libunwind, so the codebase does allow them.
…75776) Reverts llvm#125412 See the discussion in llvm#125412 for why this is necessary. The summary is that: - Eliding arguments is the C23 extension, but libunwind builds its C files with `-std=c99`, so this change broke the build. - `-Wno-unused-parameter` is part of the build for libunwind, so the codebase does allow them.
No description provided.